feat: add BrazilianCrux scale #158

enapupe · 2023-11-07T13:33:03Z

hey guys, I think this is ready to be reviewed.
My changes were based on the norwegian scale.

I did a minor "API" change in the GradeScale types to accomodate the actual list of grades, that was necessary to handle cases where a slash grade was not the next logical grade.

I suggest we start using a code formatter like prettier, life is too short to manually adjust code looks.

github-actions · 2023-11-07T13:34:08Z

Coverage report

Statements coverage not met for global: expected <=999999 not covered statements, but got 608

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	87.48% (+1.09% 🔼)	608/695
🟡	Branches	67.54% (+2.22% 🔼)	129/191
🟡	Functions	79.87% (+0.45% 🔼)	119/149
🟢	Lines	86.67% (+1.1% 🔼)	559/645

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	scales/brazilian.ts	98.33%	88.89%	91.67%	98.15%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	index.ts	100%	100%	36.84% (-2.05% 🔻)	100%

Test suite run success

268 tests passing in 17 suites.

Report generated by 🧪jest coverage report action from 05900e2

musoke

Thank you for the PR!

How familiar are you with Brazillian grades? I am not, so I will defer to you on much of this if you're a local.

I have left some comments on details inline. Here are a couple more general comments:

Name of scale

Wikipedia says that there are at least two scales used in Brazil. The one you have implemented is what they call the "Brazilian technical scale". Should the name in sandbag be something like BrazilianTechnical? This will reduce confusion if we add additional Brazilian scales later.

New field on `GradeScale`

Please don't take this as negative feedback, it's mostly me trying to understand whether we can simplfy other parts of the library.

I did a minor "API" change in the GradeScale types to accomodate the actual list of grades, that was necessary to handle cases where a slash grade was not the next logical grade.

Could we clarify why this new field is needed?

As far as I can tell you're using it to avoid duplicating array definitions in src/scales/brazilian.ts and src/index.ts. Is that correct?
If so, why are those two arrays defined? My understanding is

In src/scales/brazilian.ts, the array allows checking that slash grades are consecutive. This makes a lot of sense. Thanks for adding a check that most of the other scales are missing 😄.
In src/index.ts, you added the array to freeClimbing for consistency with other scales.

Is that correct?

There may be something we can simplify here: it isn't clear to me why we have defined the arrays in src/index.ts for other scales. @vnugent, do you have some insight here? Is

sandbag/src/index.ts

Lines 273 to 286 in d5c4170

    
           export const freeClimbing = { 
        
             clean: { 
        
               yds: YDS_ARRAY, 
        
               class: CLASS_ARRAY, 
        
               britishTech: BRITISH_TECH_ARRAY, 
        
               britishAdj: BRITISH_ADJ_ARRAY, 
        
               French: FRENCH_ARRAY, 
        
               UIAA: UIAA_ARRAY, 
        
               Ewbank: EWBANK_ARRAY, 
        
               Saxon: SAXON_ARRAY, 
        
               Norwegian: NORWAY_ARRAY 
        
             }, 
        
             community: {} 
        
           }

vestigial? These arrays aren't used internally. Maybe it is useful to expose these in the API, but the current definitions are wrong and/or missing. The analogous array bouldering is empty and ice, aid, etc aren't even defined. The current tests are nearly tautological.

Summary of sources on Brazillian grades (mostly for myself)

English wikipedia: https://en.wikipedia.org/wiki/Grade_(climbing)#Brazil
UIAA document: https://theuiaa.org/documents/mountaineering/THESCALESOFDIFFICULTYINCLIMBING_p1b.pdf
doi.org/10.1080/19346182.2015.1107081 & https://www.ircra.rocks/single-post/2016/09/12/reporting-grades-in-climbing-research the tables in the first two may be based on this
Issue where thecrag sorted out some of the same questions: brazilian grading systems theCrag/website#1139

src/data/routes.csv

src/scales/brazilian.ts

README.md

src/data/routes.csv

src/__tests__/scales/brazilian.ts

musoke · 2023-11-09T18:21:01Z

I suggest we start using a code formatter like prettier, life is too short to manually adjust code looks.

I am also a fan of code formatters. Most of OpenBeta's repos already use ts-standard - please open a separate issue or PR if you think that isn't enough.

enapupe · 2023-11-09T18:46:44Z

I suggest we start using a code formatter like prettier, life is too short to manually adjust code looks.

I am also a fan of code formatters. Most of OpenBeta's repos already use ts-standard - please open a separate issue or PR if you think that isn't enough.

To be honest I haven't paid much attention to it, I remember I quickly runned it with --fix and didn't noticed it also formatted the code besides checking for types.
I guess one code formatter is sufficient :)

enapupe · 2023-11-09T18:50:48Z

As far as I can tell you're using it to avoid duplicating array definitions in src/scales/brazilian.ts and src/index.ts. Is that correct?

Yes, exactly. I was trying to keep things consistent while not duplicating that array. The exposed API in somewhat unknown to me so that's why I did that change to access the array. Another option that crossed my mind was to actually generate that array from the json file, just getting the grades (ordered) and making it a set (i.e. removing dupes). Then it's automatically generated from within.

enapupe · 2023-11-09T18:59:45Z

Wikipedia says that there are at least two scales used in Brazil. The one you have implemented is what they call the "Brazilian technical scale". Should the name in sandbag be something like BrazilianTechnical? This will reduce confusion if we add additional Brazilian scales later.

This information may be outdated, currently there's a single scale used in Brazil. In 2007 the brazilian National Confederation of Mountaineering and Climbing has published this spec and incentivized its use as the only standard

enapupe · 2023-11-09T19:07:33Z

@musoke thank you for the throughout review! Really appreciate it.

How familiar are you with Brazillian grades? I am not, so I will defer to you on much of this if you're a local.

I'm brazilian and have been climbing around the country the last 10 years, the brazilian spec is not really that complex, but there are many misleading informations out there, especially from english sources. I think that's because in the past there was no official source of truth, but that was way before I started climbing so I can't tell for sure.

enapupe · 2023-11-09T19:15:44Z

The spec is pretty official and well established across the country.

…

On Thu, Nov 9, 2023 at 4:03 PM nathan musoke ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/data/routes.csv <#158 (comment)>: > +8,5.0,1c,1,2,1,1+,Isup +9,5.0,1c,1,2,1,1+,Isup +10,5.0,1c+,1,3,1,2-,Isup +11,5,1c+,1,4,1,2-,Isup +12,5.1,2a,2,5,2,2,Isup +13,5.1,2a,2,5,2,2,Isup the spec <https://www.cap.com.br/post/sistema-brasileiro-de-gradua%C3%A7%C3%A3o-de-vias-de-escalada> Nice, searching in English wasn't sufficient to find that. How official is this spec? Maybe there should be a link to it in the comments. Then your reasoning will be clearer to anyone who changes this code in future. — Reply to this email directly, view it on GitHub <#158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACHCCVUK5A5ASKHSJE7Y6DYDUSJPAVCNFSM6AAAAAA7BGKUJWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRTGMYDGNJSGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

musoke · 2023-11-09T21:32:22Z

Wikipedia says that there are at least two scales used in Brazil. The one you have implemented is what they call the "Brazilian technical scale". Should the name in sandbag be something like BrazilianTechnical? This will reduce confusion if we add additional Brazilian scales later.

This information may be outdated, currently there's a single scale used in Brazil. In 2007 the brazilian National Confederation of Mountaineering and Climbing has published this spec and incentivized its use as the only standard

Searching in English wasn't sufficient to find that very clear and thorough spec 😆
Thank you for the links and detailed explanation!

Could you add a link to the spec in a docstring for const Brazilian? Then the intended behaviour will be clearer to anyone who uses this or changes it in the future.

Does anyone use grades that differ from the spec in ways that matter? For example, in old guidebooks? Are there routes whose consensus grade is non-compliant?
I think we should lean descriptivist if real-world grades diverge from the spec.

musoke · 2023-11-09T21:35:42Z

As far as I can tell you're using it to avoid duplicating array definitions in src/scales/brazilian.ts and src/index.ts. Is that correct?

Yes, exactly. I was trying to keep things consistent while not duplicating that array. The exposed API in somewhat unknown to me so that's why I did that change to access the array. Another option that crossed my mind was to actually generate that array from the json file, just getting the grades (ordered) and making it a set (i.e. removing dupes). Then it's automatically generated from within.

Yes, that part of the exposed API is mysterious to me too. As far as I can tell, it is leftover from early versions. Let's keep what you wrote and address the other issues later.

enapupe · 2023-11-09T21:39:59Z

Does anyone use grades that differ from the spec in ways that matter? For example, in old guidebooks? Are there routes whose consensus grade is non-compliant?

People often mix arabic/romans for single pitch sports route, which is wrong and should not be encouraged if we want to keep things consistent/within the spec.
Aside from that, I don't have any relevant information/knowledge. I'd have to research old magazines maybe, but I'm not sure if that's relevant.

musoke · 2023-11-09T21:46:27Z

Does anyone use grades that differ from the spec in ways that matter? For example, in old guidebooks? Are there routes whose consensus grade is non-compliant?

People often mix arabic/romans for single pitch sports route, which is wrong and should not be encouraged if we want to keep things consistent/within the spec. Aside from that, I don't have any relevant information/knowledge. I'd have to research old magazines maybe, but I'm not sure if that's relevant.

Arabic -> Roman is a straightforward mapping, so likely no information loss there. Let's not worry about it if the edge cases are obscure.

musoke

This looks great! You even linked the archive.org copy of the spec.

I'm happy for you to merge.

musoke · 2023-11-09T22:41:12Z

@all-contributors add @enapupe for code and ideas

allcontributors · 2023-11-09T22:41:22Z

@musoke

I've put up a pull request to add @enapupe! 🎉

Tag and release changes from #158

[npm publish] release BrazilianCrux Tag and release changes from #158

Tag and release changes from #158

musoke linked an issue Nov 9, 2023 that may be closed by this pull request

add Brazilian grades #98

Open

5 tasks

musoke reviewed Nov 9, 2023

View reviewed changes

musoke mentioned this pull request Nov 9, 2023

Add API that canonicalises grades #159

Open

feat: add brazilian scale

05900e2

musoke mentioned this pull request Nov 9, 2023

Are arrays of possible grades supposed to be part of the API? #160

Open

8 tasks

musoke approved these changes Nov 9, 2023

View reviewed changes

enapupe merged commit d9e2059 into OpenBeta:develop Nov 9, 2023
3 checks passed

enapupe had a problem deploying to production November 9, 2023 22:35 — with GitHub Actions Failure

allcontributors bot mentioned this pull request Nov 9, 2023

docs: add enapupe as a contributor for code, and ideas #165

Merged

musoke changed the title ~~feat: add brazilian scale~~ feat: add BrazilianCrux scale Nov 9, 2023

musoke mentioned this pull request Nov 9, 2023

[npm publish] release BrazilianCrux #166

Merged

musoke added a commit that referenced this pull request Nov 9, 2023

[npm publish] release BrazilianCrux

402af20

Tag and release changes from #158

musoke mentioned this pull request Nov 9, 2023

Release brazil #167

Merged

musoke added a commit that referenced this pull request Nov 9, 2023

Release brazil (#167)

cc8045b

[npm publish] release BrazilianCrux Tag and release changes from #158

musoke added a commit that referenced this pull request Nov 9, 2023

[npm publish] release BrazilianCrux

4b76be6

Tag and release changes from #158

musoke added a commit that referenced this pull request Nov 9, 2023

[npm publish] release BrazilianCrux

7c5d4af

Tag and release changes from #158

musoke mentioned this pull request Nov 10, 2023

[npm publish] release BrazilianCrux #168

Merged

musoke mentioned this pull request Nov 10, 2023

add Brazilian grades #98

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add BrazilianCrux scale #158

feat: add BrazilianCrux scale #158

enapupe commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

musoke left a comment

musoke commented Nov 9, 2023

enapupe commented Nov 9, 2023

enapupe commented Nov 9, 2023

enapupe commented Nov 9, 2023

enapupe commented Nov 9, 2023 •

edited

Loading

enapupe commented Nov 9, 2023 via email

musoke commented Nov 9, 2023

musoke commented Nov 9, 2023

enapupe commented Nov 9, 2023

musoke commented Nov 9, 2023

musoke left a comment

musoke commented Nov 9, 2023

allcontributors bot commented Nov 9, 2023

	export const freeClimbing = {
	clean: {
	yds: YDS_ARRAY,
	class: CLASS_ARRAY,
	britishTech: BRITISH_TECH_ARRAY,
	britishAdj: BRITISH_ADJ_ARRAY,
	French: FRENCH_ARRAY,
	UIAA: UIAA_ARRAY,
	Ewbank: EWBANK_ARRAY,
	Saxon: SAXON_ARRAY,
	Norwegian: NORWAY_ARRAY
	},
	community: {}
	}

feat: add BrazilianCrux scale #158

feat: add BrazilianCrux scale #158

Conversation

enapupe commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 • edited Loading

Coverage report

Test suite run success

musoke left a comment

Choose a reason for hiding this comment

Name of scale

New field on GradeScale

Summary of sources on Brazillian grades (mostly for myself)

musoke commented Nov 9, 2023

enapupe commented Nov 9, 2023

enapupe commented Nov 9, 2023

enapupe commented Nov 9, 2023

enapupe commented Nov 9, 2023 • edited Loading

enapupe commented Nov 9, 2023 via email

musoke commented Nov 9, 2023

musoke commented Nov 9, 2023

enapupe commented Nov 9, 2023

musoke commented Nov 9, 2023

musoke left a comment

Choose a reason for hiding this comment

musoke commented Nov 9, 2023

allcontributors bot commented Nov 9, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

New field on `GradeScale`

enapupe commented Nov 9, 2023 •

edited

Loading